Skip to content

1.x: new fromAsync to bridge the callback world with the reactive#4179

Merged
akarnokd merged 3 commits intoReactiveX:1.xfrom
akarnokd:FromAsync
Jul 10, 2016
Merged

1.x: new fromAsync to bridge the callback world with the reactive#4179
akarnokd merged 3 commits intoReactiveX:1.xfrom
akarnokd:FromAsync

Conversation

@akarnokd
Copy link
Copy Markdown
Member

@akarnokd akarnokd commented Jul 8, 2016

This PR adds a new source operator: fromAsync() that let's bridge the callback-style world with the reactive world by providing a push surface and offers options to handle backpressure.

@akarnokd
Copy link
Copy Markdown
Member Author

akarnokd commented Jul 8, 2016

These errors, i.e., Travis running out of memory and killing tests, gets annoying...

}
}

}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really awesome tests. Easy to read and understand.

@akarnokd
Copy link
Copy Markdown
Member Author

akarnokd commented Jul 8, 2016

Updated.

@JakeWharton
Copy link
Copy Markdown
Contributor

LGTM 👍 !

I'm surprised you couldn't reuse more of the existing infrastructure for the various backpressure modes here and instead need to have a full implementation of the whole queue/drain stuff.

@akarnokd
Copy link
Copy Markdown
Member Author

akarnokd commented Jul 8, 2016

The codebase spans over several years now and there is no current "best toolset" for building operators. Besides, this inline saves allocation and overhead from applying other operators.

@akarnokd akarnokd added this to the 1.2 milestone Jul 8, 2016
@akarnokd
Copy link
Copy Markdown
Member Author

akarnokd commented Jul 8, 2016

/cc @stevegury @zsxwing as this is a new operator proposed to the public API


t.add(emitter);
t.setProducer(emitter);
asyncEmitter.call(emitter);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't invoking this be deferred until request > 0?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would we do that? The subscribe() itself triggers the execution of the body that sets up the push source.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, I guess it depends on what the implementation of the emitter is. I have a few use cases in my head I'm thinking through. Besides you could use AsyncOnSubscribe if you need that request coordination.

@akarnokd
Copy link
Copy Markdown
Member Author

akarnokd commented Jul 9, 2016

@JakeWharton how eager are you about this. I'd really love to merge all remaining PRs so 1.1.7 is as complete API-vise as possible. Otherwise, we may have to wait till 1.1.8 and 1.2 is also delayed.

@JakeWharton
Copy link
Copy Markdown
Contributor

I would, of course, prefer that it made it. Releases are few and far between here so missing the boat might mean 3 months before it sees the light of day.

That said, if no one from Netflix is available to review the API and it's the only thing blocking 1.1.7 then I'm fine with it missing the boat.

@akarnokd
Copy link
Copy Markdown
Member Author

I see low risk as this is a completely new operator.

@akarnokd akarnokd merged commit 6b47b11 into ReactiveX:1.x Jul 10, 2016
@akarnokd akarnokd deleted the FromAsync branch July 10, 2016 06:36
@JakeWharton
Copy link
Copy Markdown
Contributor

Thanks!

On Sun, Jul 10, 2016, 2:36 AM David Karnok notifications@github.com wrote:

Merged #4179 #4179.


You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
#4179 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AAEEETB8Xs_T7eEi0pq4xF0STz3VozPxks5qUJLcgaJpZM4JIVRj
.

@defHLT
Copy link
Copy Markdown

defHLT commented Jul 12, 2016

I wonder should this method be used instead of create in cases like RxBinding present?

@stevegury
Copy link
Copy Markdown
Member

👍
Sorry for the late response (I was on vacation)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants